FLUTE v1 (RFC 3926) and MBMS Download Profile conformance, plus assorted correctness fixes - #62
Conversation
52a3ed8 to
a9b4650
Compare
a9b4650 to
d3b8792
Compare
Spec-compliance verificationEach item below was checked directly against the primary RFC text (not inferred from existing code comments or secondary sources):
|
|
3GPP TS 26.517 (v18.5.1)
3GPP TS 26.346 (v18.1.0)
Therefore the FLUTE session for 3GPP applications is based on RFC3926, which is the FLUTE v1 specification, and states:
Since we use this library with 3GPP applications it needs to support version 1 of FLUTE. It could support version 2 as well, but changes to make this version 2 only will break 3GPP based applications. |
davidjwbbc
left a comment
There was a problem hiding this comment.
I have no problem with FLUET v2 being added, but this library needs to support FLUTE v1 for compliance with 3GPP specifications. Please do not make changes that remove FLUTE v1 compliance.
|
Thanks @davidjwbbc. Having discussed this now wth @dsilhavy, I'm going to defer v2 to another branch and leave that code out of this PR. Then I will check that all v1 is consistent with 3GPP and update this PR accordingly. |
AlcPacket's transmit constructor took a uint16_t TSI while Transmitter itself stores a uint64_t, silently truncating any TSI above 65535 on every packet. Widened the constructor to uint64_t and now sets tsi_flag (the LCT header's S flag, adding a second 32-bit TSI word per RFC 5651 SS4.2's S/H flag mechanism -- unchanged since RFC 3451, so this applies identically to FLUTE v1) whenever the value exceeds 16 bits, giving the full 48-bit field width this wire scheme supports. Values that fit in 16 bits keep the original single-half-word encoding unchanged. Dropped this commit's original FLUTE-version-2 change: RFC 6726 SS3.1/3.4.1 does require version 2, but 3GPP TS 26.517 cl.6.2.1 -> TS 26.346 Annex L mandates the FLUTE v1 profile (RFC 3926) for MBMS/5G-MAG applications, and this library's primary consumers are exactly those applications -- per davidjwbbc's review on 5G-MAG#62. Kept on a separate future/flute-v2-support branch in case genuine (non-3GPP) FLUTE v2 support is wanted later.
d3b8792 to
6c7137d
Compare
|
Pushed a corrected version, per this thread:
Tests/build unaffected — only the one line (FLUTE version nibble) and its commit message changed; the other three commits are untouched. |
|
@jordijoangimenez: Now the scope of this PR has changed to be compliance with TS 26.517 and TS 26.346, the title should also be changed accordingly. Given that the target is now compliance with FLUTE v1, we are talking about RFC 3986 rather than RFC 6726. Rather than burying the update in a supplementary comment, it would be better to integrate the correct scope into the main issue description to avoid confusing future readers. |
RFC 6726 SS3.1/SS3.4.1 requires version 2 in EXT_FDT; the decoder already tolerates up to version 2 (unchanged by this commit), only transmit lagged. Split out of 5G-MAG#62: 3GPP TS 26.517 cl.6.2.1 -> TS 26.346 Annex L mandates the RFC 3926 (v1) profile for MBMS/5G-MAG applications, this library's primary consumers, so this must not be the default. Kept as a separate branch for a genuine non-3GPP FLUTE v2 deployment, or in case 3GPP adopts v2 later.
|
Suggestions applied, @rjb1000 |
Problem On completing any file, the receiver erases every other file carrying the same Content-Location, so that a re-sent object replaces the copy already in progress. A file started from a content packet's own EXT_FTI has no Content-Location until its FDT entry arrives, and neither does the transient file the TOI 0 FDT itself is reassembled into. The two empty placeholders compared equal, so completing the FDT erased the bootstrapped file immediately before the loop that would have given it its metadata. src/Receiver.cpp:264. [code-derived] Basis RFC 3926 clause 5: "For the TOI values other than 0 the receiver MUST support both methods: the use of EXT_FTI and the use of FDT." That support is what the erased file was carrying out. No clause governs the replacement sweep itself; it is this library's own de-duplication, and the identity it keys on is a Content-Location, which an empty string is not. Raised by reading the authority during this work Change The sweep now skips the comparison when the completed file's Content-Location is empty. The receiver owns both the sweep and the bootstrap, so the fix belongs there. Behaviour for a real repeated Content-Location is unchanged. Verification T1: protocol:ExtFtiBootstrapTest passes, 3 cases, the third of which fails without this change. Whole suite 49 cases, all passing. Not in this change Nothing about what the receiver does when the FDT never arrives; such a file keeps an empty Content-Location and is not written out, which is unchanged.
Problem configure_state() attached one algorithm attribute, XFRMA_ALG_CRYPT carrying aes, so the security association enable_ipsec() installs offers confidentiality and no integrity. A receiver on such a session cannot distinguish a sender's packet from a forged one, which is usually the property a caller reaching for IPsec on a multicast session wants. Observed: ip xfrm state show on an association installed by enable_ipsec() shows an enc line and no auth line. [observed] Basis RFC 4303 clause 1: "Using encryption without a strong integrity mechanism on top of it (either in ESP or separately via AH) may render the confidentiality service insecure against some forms of active attacks" The same clause lists confidentiality with integrity as a MUST for an ESP implementation and confidentiality alone as a MAY. RFC 3926 clause 7: "At the packet level, it is RECOMMENDED that a packet level authentication be used to ensure that each received packet is an authentic and uncorrupted packet containing FEC data for the object arriving from the specified sender." IPsec is the only packet-level protection this library offers, so an encryption-only association leaves that recommendation unmet even where a caller has enabled it. Neither document names an algorithm. HMAC-SHA256 is an engineering choice. Raised by reading the authority during this work Change IpSec owns the association, so it is fixed there. configure_state() attaches XFRMA_ALG_AUTH with hmac(sha256) alongside the encryption algorithm. enable_esp(), Transmitter::enable_ipsec() and Receiver::enable_ipsec() take an authentication key. A caller supplying only an encryption key keeps working: the authentication key is then derived from it through SHA-256 with a fixed context string, which gives the two algorithms distinct key bytes. That is weaker than two independent keys, which is why the parameter exists. Verification T2: with the two-key call, ip xfrm state show reports "auth-trunc hmac(sha256) 0x1112131415161718191a1b1c1d1e1f20 96" beside the enc line, the key exactly as supplied. With the one-key call it reports a derived key differing from the encryption key. Test associations flushed afterwards. Suite unaffected, 49 cases passing. Not in this change The truncation length is Linux's 96-bit default for XFRMA_ALG_AUTH rather than RFC 4868's 128-bit HMAC-SHA-256-128; changing it needs XFRMA_ALG_AUTH_TRUNC and matters only against an external peer expecting the longer form. No anti-replay window is configured.
Problem With a tunnel endpoint configured the transmitter builds the inner IP and UDP headers itself. Both builders took the addresses with to_v4(), which throws bad_address_cast for a v6 address, so an IPv6 destination did not degrade, it threw out of the send path on the first packet. Four sites also subtracted or added a literal 20 for the IP header, which is the IPv4 size, so an IPv6 session set a payload 20 bytes over the budget the caller asked for, tunnelled or not. Observed: the new test aborts with "terminate called after throwing an instance of boost::asio::ip::bad_address_cast" against the unchanged code. [observed] Basis RFC 8200 clause 8.3: "an upper-layer protocol must take into account the larger size of the IPv6 header relative to the IPv4 header." RFC 8200 clause 8.1: "whenever originating a UDP packet, an IPv6 node must compute a UDP checksum over the packet and the pseudo-header, and, if that computation yields a result of zero, it must be changed to hex FFFF for placement in the UDP header." Clause 3 gives the header format and clause 8.1 the pseudo-header the checksum covers. Raised by reading the authority during this work Change Transmitter owns the encapsulation, so it is fixed there. create_ip_hdr() writes a 40-byte IPv6 header for a v6 destination, create_udp_pkt() computes the checksum over the clause 8.1 pseudo-header and substitutes all ones for a computed zero, and the four budget sites take the header length from the destination's family through one helper. IPv4 behaviour is unchanged, and the untunnelled path, where the kernel builds the headers, is untouched. Verification T1: unit:TransmitterIPv6TunnelTest passes, 1 case. It captures the bytes a real Transmitter sends to a local socket standing in for the tunnel peer, and checks the version nibble, payload length, next header and destination address, then re-derives the UDP checksum over the pseudo-header with its own summation and confirms it self-verifies to zero. Reverting the change makes it abort as quoted above. Whole suite 50 cases, all passing. Not in this change No IPv6 extension headers are written; the fixed 40-byte header is all this path needs. The IPv4 fragmentation and identification fields keep their existing zero values.
… tunnel Problem With a tunnel endpoint configured, send_next_packet() chose between the tunnel and the announced destination rather than using both, so a receiver that joins the announced address directly, instead of sitting behind the tunnel's decapsulation, received nothing at all. The session looked healthy from the sender: packets were transmitted continuously and reached the tunnel peer. src/Transmitter.cpp, the if/else on _tunnel_endpoint in send_next_packet(). [code-derived] Basis No clause governs this. A tunnel is a transport for the same session, not a different destination, and nothing in the FLUTE configuration says that setting one withdraws the announced address. Engineering choice, see rule 12. Raised by reading the authority during this work Change Transmitter owns the send path, so it is fixed there. When a tunnel is configured both copies now go out: the plain copy fire-and-forget, and the encapsulated copy carrying the completion bookkeeping, which preserves the completion timing tunnelled operation already had. Without a tunnel the single plain send carries it, as before. This is also the behaviour the earlier commit "transmitter: bind to the configured source address even when tunnelling" was written for: binding the source address matters precisely because a tunnelled session also emits an untunnelled copy, which until now it did not. Verification T1: whole suite passes, 50 cases, including e2e:FluteEndToEndTest.TransmitsFileToReceiverThroughUdpTunnel and unit:TransmitterIPv6TunnelTest, which both exercise the tunnelled path. Not in this change No way to ask for the tunnelled copy only. If a deployment needs to suppress the plain copy that is a new option, not a change to this default.
0545e9d to
637cf46
Compare
…ach to its own rules Problem One profile value stood for two different 3GPP profiles that mandate different FDT schemas with different mandatory schemaVersion values, and the schema was a separate argument that could contradict it. A default-constructed 3GPP session emitted an FDT with no namespace at all, conforming to neither schema; a session on the clause 7.2.10.1 schema never emitted the schemaVersion that schema requires; and a TSI wider than the field the specifications fix was widened rather than refused. Three further obligations were unimplemented: the ceiling on Maximum-Source-Block-Length, the precedence of File@Expires over FDT@Expires, and the prohibition on using an expired FDT Instance. [source-derived] Basis TS 26.517 V18.6.0 clause 6.2.1: "The MBSTF shall use the Profiled FDT Schema according to clause L.6 of TS 26.346 [7] to describe the object list currently being transmitted in the MBS Distribution Session." TS 26.346 V18.2.0 clause 7.2.9: "The extended FLUTE FDT instance schema defined in clause 7.2.10.1 (based on the one in RFC 3926 [9]) shall be used." TS 26.346 V18.2.0 clause 7.2.10.1: "In this version of the present document the network shall set the content of the schemaVersion element, defined as a child of the FDT-Instance element, to the value 4." TS 26.346 V18.2.0 clause L.6.3: "The BM-SC shall set the schemaVersion element to 2 in all instance documents" TS 26.346 V18.2.0 clause 7.2.7: "-The Transmission Session Identifier (TSI) field shall be of length 16 bits (S=0, H=1, 16 bits)." TS 26.346 V18.2.0 clause 7.2.9: "For MBMS operation, the UE shall not use a received FDT Instance to interpret packets received beyond the expiration time of the FDT Instance." Raised by reading the authority during this work Change Profile becomes Mbs5g, MbmsDownload and GeneralFlute, defaulting to Mbs5g, and each 3GPP profile derives its own FDT namespace so the two can no longer disagree. schemaVersion is emitted per schema, 2 for the annex L.6.1 one and 4 for the clause 7.2.10.1 one. Every 3GPP restriction now tests is_3gpp() rather than naming a single profile, so the annex L.4 rules apply to both, which they always did in the specification. Added: refusal of a TSI above 65535 and of a Compact No-Code Maximum-Source-Block-Length above 65535 under either 3GPP profile; effective_expiry(), applying File@Expires over FDT@Expires; and expired(), which the receiver now consults before interpreting any packet against a held FDT Instance. Comments and messages naming annex L's profile for what are clause 7.2 rules corrected. Verification T1: 61 cases pass, 10 new, covering the schema and schemaVersion each profile produces, the namespace argument being overridden by a 3GPP profile, both refusals at and above their ceilings, the expiry precedence in both directions, and the expiry boundary. Not in this change Nothing above the mandatory level. The should and may items of clause 7.2 and annex L are assessed but not implemented, and none of the general-FLUTE RFC obligations are touched.
e6a82db to
c7306fc
Compare
Problem set_expires() accepted any value, including a time already past. An instance created already expired can never be used to interpret anything, and a receiver following TS 26.346 clause 7.2.9 discards it the moment it arrives, so the session transmits and delivers nothing. The library's own Transmitter always passes a future time, so this was reachable only through the public setter. [source-derived] Basis RFC 3926 clause 3.3: "A sender MUST use an expiry time in the future upon creation of an FDT Instance relative to its Sender Current Time (SCT)." Binding under every profile, not only the 3GPP ones, which inherit it through TS 26.346 clause 7.2.0's adoption of RFC 3926. Raised by reading the authority during this work Change FileDeliveryTable owns the attribute, so the check is there. set_expires() becomes a real function and refuses a value at or before the current time. The parse path is untouched: it assigns the member directly, and a receiver must be able to read an instance that expired in transit in order to discard it. Verification T1: 63 cases pass, 2 new, covering refusal under a 3GPP profile and under plain RFC 3926, and acceptance of a future value. Two existing expiry tests used a fixed constant of 1000 as an expiry; they now compute a real future NTP time, which is what a sender does. Not in this change No check that the expiry is far enough ahead to be useful. The clause requires only that it be in the future, and how far is the operator's choice.
4c3e556 to
e6bbe48
Compare
…e FEC Instance ID field Problem Two defects in the EXT_CENC and EXT_FTI parsing, both found by reading RFC 3926 as prose rather than searching it for mandatory keywords. An EXT_CENC naming a content encoding outside the four the library knows fell through the switch and left the encoding at NONE, so the undecoded FDT bytes went to the XML parser as though they were XML. The failure then surfaced somewhere further on, naming the wrong thing. Separately, the 16 bits between the transfer length and the encoding symbol length were commented as reserved on both the parse and the send side. They are the FEC Instance ID. [code-derived] Basis RFC 3926 clause 3.4.3, on why an unknown value cannot be ignored: "The definition of this field is outside the scope of this specification." RFC 3926 clause 5.1.1, on the field that is not reserved: "It is only present if the value of FEC Encoding ID is in the range of 128-255. When the value of FEC Encoding ID is in the range of 0-127, this field is set to 0." Raised by reading the authority during this work Change An unrecognised CENC value is refused with a message naming it, rather than treated as no encoding. The comments on both sides now name the FEC Instance ID and cite why a Fully-Specified scheme leaves it zero. No wire behaviour changes for any encoding the library does know, and the send side already wrote the zero the clause requires. Verification T1: 63 cases pass, unchanged. No new case: reaching the refusal needs a hand-built packet carrying an EXT_CENC with an out-of-range value, and the existing header-parse tests cover the extension walk that leads to it. The change is a refusal on a path that previously produced a misleading error further on, so a test would assert which of two errors is raised. Not in this change Nothing decodes an encoding beyond the four already supported.
Problem A packet was accepted on a TSI match alone. The arriving source address was available, held in _sender_endpoint by every receive, and never compared with anything. A session is identified by the source and TSI together, so on an any-source join any sender on the group using the session's TSI was accepted as though it were the session's own, and its packets fed into object reassembly. [source-derived] Basis RFC 3450 clause 4.5: "The receiver MUST verify that the sender IP address together with the TSI carried in the header matches one of the (sender IP address, TSI) pairs that was received in a Session Description and that the receiver is currently joined to." The same clause continues that on no match "the packet MUST be discarded without further processing", which is what this does, and it orders the receiver's steps so that this one precedes any processing of the packet. RFC 3451 clause 6.2 is what made the ordering matter: a receiver unable to process an LCT header "MUST drop from the session", so whether a parse failure is a session-level fact or a stray datagram depends on having ruled the datagram out first. Raised by reading the authority during this work Change Receiver holds the socket and the session identity, so the check belongs there. Where the caller named a source it is parsed once at construction, and every datagram's origin is compared against it before the packet is parsed at all, so traffic that is not this session's cannot reach the parser or influence how a parse failure is treated. A mismatch is discarded with the source logged. A source-specific join already has the kernel filtering on the same address, so there the check is defence in depth against a routing or membership mistake rather than the only guard. For an any-source session the library is given no source to compare against and the obligation cannot be met at all; that is recorded as a limitation rather than passed over, and a caller that wants it should name the source. Verification T1: 63 cases pass, unchanged. No new case: reaching the discard needs a second sender on the same group and port as an existing source-specific session, which the test scaffolding cannot stand up without a second host or a routing fixture. Not in this change Clause 4.5's "If multiple packets are received with non-matching (sender IP address, TSI) values then the receiver SHOULD leave the session" is not implemented; nothing counts mismatches. Nor is clause 6.2's "MUST drop from the session" on an unprocessable header: the library discards and continues. A receiver that leaves on a burst of foreign or malformed packets is easier to disrupt than one that discards them, so both are deliberate and recorded rather than overlooked.
4435e3b to
bdb6b09
Compare
bdb6b09 to
44ad0f0
Compare
…es for Problem close_session() sets a flag on packets sent from that point on, so on a session with nothing left to send it signals nothing at all. Once the file set empties, send_fdt() returns early and no further packet is ever built. src/Transmitter.cpp, close_session(). [code-derived] Basis RFC 3450 clause 4.1: "In some special cases an ALC sender may need to produce ALC packets that do not contain any payload. This may be required, for example, to signal the end of a session or to convey congestion control information." RFC 3926 clause 3.1 gives the packet its shape in a FLUTE session: "the exception that ALC packets sent in a FLUTE session with the Close Session (A) flag set to 1 (signaling the end of the session) and that contain no payload (carrying no information for any file or FDT) SHALL NOT carry the TOI" Dropping the TOI drops the half-word flag the two fields share, so the TSI becomes whole words. RFC 5651 clause 5.1: "The TSI field is 32*S + 16*H bits in length" On the receiving side the flag is advisory, which is why the packet is sent once and not retransmitted. RFC 5651 clause 5.1: "the receiver SHOULD assume that no more packets will be sent to the session" Raised by reading the authority during this work Change AlcPacket gains a constructor for a header-only Close Session packet: no payload, so no FEC Payload ID and no TOI. Transmitter sends one when close_session() finds the queue already empty, and one more when the last file drains while the session is closing. Both send paths are covered, plain and tunnelled, fire and forget. A TSI wider than 32 bits cannot express this packet, since the whole-word TSI this encoding builds is one word. It is refused rather than emitting the TOI the clause forbids, and the transmitter logs and carries on rather than failing the session. Verification T1: flute_protocol_tests, 66 cases in the suite pass, three new: the header's every field against the clause, the refusal above its TSI ceiling, and a round trip back through this library's own parser. Not in this change Retransmitting it, or repeating it over the last few seconds of the session, which clause 5.1 offers as a SHOULD once the flag has been set in one packet. Sending it on a session whose TSI needs more than 32 bits, which the encoding cannot express: recorded as a limitation.
Problem A packet with no payload restarted FDT reception and reassembled a zero-length object from it. A header with no TOI decodes to TOI 0, so such a packet entered the TOI 0 branch and replaced the object in progress. Observed: with the guards removed, a data-less packet delivered to a live receiver leaves a TOI 0 entry in file_list(). [observed] Reachable beyond that, and not demonstrated here: with an FDT object present and incomplete while the held instance id matches, src/EncodingSymbol.cpp subtracts a four-byte FEC Payload ID from a payload length of zero, which wraps a size_t. [code-derived] Basis RFC 3450 clause 4.1: "In some special cases an ALC sender may need to produce ALC packets that do not contain any payload." RFC 3450 clause 4.1, on how a receiver tells: "The total datagram length, conveyed by outer protocol headers (e.g., the IP or UDP header), enables receivers to detect the absence of the ALC payload and FEC Payload ID." RFC 3926 clause 3.1 gives the shape a FLUTE sender uses, which is why a conformant peer sends one: "the exception that ALC packets sent in a FLUTE session with the Close Session (A) flag set to 1 (signaling the end of the session) and that contain no payload (carrying no information for any file or FDT) SHALL NOT carry the TOI" RFC 3450 clause 4.5 puts the length check in step 1, before the payload is touched: "The receiver MUST parse the packet header and verify that it is a valid header. If it is not valid then the packet MUST be discarded without further processing." Raised by reading the authority during this work Change Receiver measures the payload from the datagram length, as the clause says to, and returns after honouring the Close flags when there is none. A payload present but shorter than a FEC Payload ID is discarded as an invalid packet. Receiver owns this because the length is only known where the datagram is; AlcPacket sees the header alone. Verification T1: flute_protocol_tests, 67 cases in the suite pass, one new. Removing both guards makes the new case fail on the TOI 0 entry, so it tests the fix rather than passing regardless. Not in this change Distinguishing an absent TOI from TOI 0 in AlcPacket's own interface. The payload length settles this case, and no other caller needs the distinction yet.
…tination alone Problem The security policy selector named only the destination address, so it captured every datagram addressed to that group whatever it was for: another session sharing the group on a different port, or any other protocol addressed there, was pushed through this session's association. src/IpSec.cpp, configure_policy(): neither sel.proto nor sel.dport was ever set. [code-derived] Basis No version 1 document governs this: RFC 3450 contains no IPsec baseline at all. The obligation belongs to version 2, which inherits it. RFC 6726 clause 7.5: "Since FLUTE relies on ALC/LCT, it inherits the "baseline secure ALC operation" of [RFC5775]." RFC 5775 clause 5.1.1: "The sender IPsec SPD entry MUST be configured to process outbound packets to the destination address and UDP port number of the applicable ALC session." RFC 5775 clause 5.1.2.1: "The implementation MUST be able to use the source address, destination address, protocol (UDP), and UDP port numbers as selectors in the SPD." It lands here rather than on the version 2 branch because the library has one enable_ipsec() and no per-version IPsec path, so the over-broad selector was over-broad for both versions. For version 1 this is a defect fix; for version 2 it is also a conformance fix. Raised by reading the authority during this work Change configure_policy() sets the protocol to UDP and the destination port to the session's own, with a full port mask. enable_esp() takes the port, and both callers pass the port they already hold: Transmitter from its endpoint, Receiver from a new member holding the constructor's argument. The source port is deliberately not selected on. A sender's source port is not part of the session description, so pinning it would exclude legitimate traffic. Verification T2: installed against the running kernel and read back with `ip xfrm policy show`. The policy reads `src 0.0.0.0/0 dst 239.255.42.7/32 proto udp dport 40085`, `dir out`, `mode transport`. Reproducible by the short program in the pull request description. Test policies and states flushed afterwards. T1 alongside: 67 cases in the suite pass, none new; installing a policy needs CAP_NET_ADMIN, so this path has no test scaffolding. Not in this change A selector on the source address, which the same clause lists as available but does not require, and which the library cannot fill for an any-source session.
Problem `tests/test_transmitter.cpp` named RFC 8200 clause 3 in a comment and then asserted against it with an unquoted failure message on the following line, which the citation checker reads as the clause's text and cannot confirm. A reader has no sentence to check the assertion against either. [code-derived] Basis RFC 8200 clause 3: "Version 4-bit Internet Protocol version number = 6." Raised by reading the authority during this work, prompted by `tools/verify-citations.py` reporting the line as an unconfirmed citation on every branch in the stack Change The clause reference and its sentence are set out above the assertions, in the shape the checker reads and a reader can follow. The gtest failure message no longer doubles as a quotation. Comment and test-message only. No behaviour changes and no assertion changes. Verification T1: 67 cases pass, unchanged. The citation checker reports 58 confirmed and none unconfirmed across this branch's sources, where it previously reported one failure. Not in this change The other assertions in the same test, whose fields the same clause defines and which the comment now covers collectively rather than one quotation each.
44ad0f0 to
4c164ce
Compare
|
You've been busy this weekend, @jordijoangimenez! If I have understood your deckchair rearrangement correctly, you have closed the following Pull Requests and adopted the changes onto this PR. If that's correct, the bug issues fixed by the adopted PRs need to be explicitly listed in the main description of this PR so that they are automatically closed when this PR is (eventually) merged and closed. Raptor10 AL-FEC fixes
However, maybe these would be better adopted by PR #61 instead (intended to be merged after this PR) to keep all the Raptor10 changes together on that PR, and to avoid over-bloating this one? Other bugfixes
|
|
@rjb1000 you read it correctly, and the list below is the explicit mapping you asked for. Nothing was dropped and nothing is waiting to be re-adopted. This pull request: merge it first, on its ownStatus: ready. Nothing blocks it. 38 commits, 19 files, +2390/-127. 67 cases pass. All 58 citations in its sources verify against the specification text. What to do
What is in it: FLUTE version 1 (RFC 3926) and the MBMS Download Profile of TS 26.346 annex L, the three-profile model, and the assorted correctness fixes listed in the description. It closes thirteen issues. Ignore the branch name. One decision for you: whether #61 should take How this set of pull requests came to look like this
Where the nine closed pull requests went
One correction, @rjb1000. #93 is listed above as "already merged in PR #60". #60 merged the FDT growth fix on 17 August and changed no test file; Every open issue, and which pull request closes it
Four open issues are deliberately closed by nothing:
The stack, and the order to merge it inGitHub retargets each child to One trap for whoever merges. The heads live in two repositories: #61, #68 and #98 in |
Closes #70, closes #71, closes #72, closes #73, closes #74, closes #75, closes #76, closes #82, closes #83, closes #85, closes #96, closes #97, closes #27.
For reviewers: what changed since you last looked
Ignore the branch name. It is a historical artifact and describes nothing in this pull request. It
reads
feature/wave3-rfc6726-compliance-fixes: "wave3" was internal shorthand that means nothing outsideour own notes, and RFC 6726 is FLUTE version 2, which this branch does not implement and never should
have claimed. What is actually here is FLUTE version 1 (RFC 3926) and the MBMS Download Profile of
TS 26.346 annex L, which is what the title and the sections below describe. The name stays only because
renaming the head branch of an open pull request closes it, and closing this one would discard the review
history on it. It will be renamed after merge.
The review comments on this pull request have all been acted on:
do not make changes that remove FLUTE v1 compliance." Done. All FLUTE version 2 material is out of this
branch and lives in FLUTE version 2 (RFC 6726) signalling, off by default: step one of three, no conformance claimed #65, which is deliberately non-default and claims no conformance. TS 26.517
clause 6.2.1 points at TS 26.346 annex L, which is scoped to RFC 3926 throughout, so version 1 is the
only thing this branch implements.
and more thoroughly than at the time: fourteen citations in the code were corrected from the
RFC 6726/5651/5775 generation to RFC 3926/3450/3451, and two commit messages that still named the wrong
generation were corrected since. Where a rule genuinely exists only in the later document, the commit now
says so rather than implying a version 1 obligation: RFC 3926 states no constraint on reusing an FDT
Instance ID, so that rule is cited to RFC 6726 and adopted as sound practice, not as a requirement.
confusing future readers." Done. The scope is in the title and in this description, not in a trailing
comment.
this branch is rebased onto current
development, which includes Feature/end to end test tunneled mode #56, Fix unbounded FDT growth (same-TOI resend, and content-change cases) #60 and Bug-fix: Fix incorrect function call during FLUTE object compression #69.should review the pull requests." All five are drafts, and the order is at the end of this description.
What was checked before asking you to look again
developmentby reading the code, not inferred froma report or a note.
number was checked separately by finding the quoted sentence and reading back to its enclosing heading.
The clause number is the part a quotation checker cannot verify, and two locators were wrong until that
pass caught them.
sniffer that links none of this library's code.
Dependencies and issues
Depends on: nothing in this repository. This is the base of the set and merges first.
Blocks: #61, #64, #65, #68 and #98, all of which build on it.
Closes on merge: #70, #71, #72, #73, #74, #75, #76, #82, #83, #85, #96, #97, #27.
Cross-repository impact. Four repositories in the organisation vendor rt-libflute. None of them
is source-broken by this, which I checked rather than assumed after first claiming otherwise.
rt-mbs-transport-function: compiles unchanged, one redundancy left behind. It passes
FileDeliveryTable::FDT_NS_3GPP_CONSOLIDATED_V2explicitly inObjectCarouselPackager.ccandObjectListPackager.cc, and references noProfile::value anywhere. That enum constant still exists,and the default profile
Profile::Ts26517now derives the same schema, so the argument is overriddento exactly the value it already asked for. Behaviour is identical. The argument and the comment above
it are now redundant, and the comment is mildly misleading because it implies the caller chooses the
schema.
Do not simplify them before this merges. Dropping the argument to
FDT_NS_NONEis only correctagainst a library that has this change. Against the version MBSTF's subproject currently pins, which
predates it,
FDT_NS_NONEmeans no namespace at all, so the tidy-up would make the MBSTF emit anamespace-less FDT. I tried it, found exactly that, and reverted. Leaving the explicit argument is
safe against both versions: the old library honours it and this one overrides it to the same value.
Worth doing once MBSTF's submodule has moved, not before.
rt-mbms-mw: compiles unchanged, behaviour changes. It constructs only the five-argument
LibFlute::Receiver. But this branch makes the receiver stop using an expired FDT Instance, perTS 26.346 clause 7.2.9, where it previously kept using one indefinitely. A deployment whose sender
emits short
Expiresvalues will see reception behave differently. It tracks branchmain, so nothingreaches it until
mainis updated.rt-mbs-client, rt-mbms-examples: no affected API found. Both vendor the library; neither references
the profile, the FDT namespace or the
Transmitterconstructor in the source scanned. That scan coveredthe first sixty source files of each repository rather than the whole tree, so it is good evidence, not
proof.
What this is
The correctness and conformance baseline every other pull request in this set sits on, and the branch
each of them is based on. 32 commits, 19 files, 2,005 added lines. Three parts:
Part one, conformance. The library could not be operated so that its FDT conformed to the profile its
primary consumers are bound by, and its receive-side packet parse had four defects, two of them reachable
from a single unauthenticated datagram.
Transfer-Lengthwithheld,Content-Encodingrestricted to gzip, the profiled schema's mandatoryschemaVersionemitted, the FileExpiresattribute separated from theCache-ControldirectiveFEC-OTI-FEC-Instance-IDno longer emitted for a Fully-Specified FEC scheme, guarded on the scheme rather than the profile, because it is equally wrong outside the profileCompleteattribute and the LCT Close Session and Close Object flags, on both sides, with a callbackctestruns the suite instead of reporting success having found nothingPart two, correctness fixes that are not FEC and not tunnelling. These were buried inside the Raptor
branch and belong here, with general library correctness.
IpSec.hgains the<cstdint>it had been getting transitivelyContent-Locationcannot decide where they are writtenIt also adds regression coverage for the FDT growth fixes merged in #60, which had none, and the read-only
Transmitter::fdt()accessor those tests need.A profile flag decides which obligations apply, so a general FLUTE session keeps its previous behaviour
wherever plain RFC 3926 permits it. That flag is the mechanism the rest depends on, not a fix of its own.
Baseline
TS 26.346 V18.2.0, TS 26.517 V18.6.0, RFC 3926, RFC 3450, RFC 3451, RFC 5052, and RFC 6726 only where it
is the sole source for a rule. Each commit quotes the sentence it rests on. Note for reviewers who saw the
earlier version: the citations were corrected from the RFC 6726/5651/5775 generation to RFC 3926/3450/3451,
per @rjb1000's review.
Part three: the profile model, and the 3GPP mandatory sweep
Added after a clause-by-clause read of TS 26.346 clause 7.2, TS 26.346 annex L and TS 26.517 clause 6.2.
Scope was set deliberately at mandatory obligations of the 3GPP profiles only; "should" and "may"
items were assessed and left unimplemented, and the general-FLUTE RFC layer was not swept.
Profilenow has three values, named after the governing documents. Only one of the three profileshas a name in the specifications at all: annex L.4 is titled "MBMS Download Profile", TS 26.517's variant
is described in clause 6.2.1 only as that profile "with the additional requirements in clause 6.2 of the
present document", and plain FLUTE has no name because it is the absence of a profile. So the enum names
documents rather than inventing profile names.
schemaVersionTs26517(default)urn:3GPP:metadata:2022:FLUTE:FDTTs26346urn:IETF:metadata:2005:FLUTE:FDTUnprofiledThe profile now derives the FDT schema. It used to be an independent argument that could contradict
the profile, and a default-constructed 3GPP session emitted an FDT with no namespace at all,
conforming to neither schema.
Gaps closed by the sweep, each with its clause quoted at the code:
schemaVersion4 never emitted for the clause 7.2.10.1 schemaFEC-OTI-Maximum-Source-Block-Lengthfor Compact No-CodeFile@Expiresnot taking precedence overFDT@ExpiresThe profile is now a hard boundary. gzip, RaptorQ and FLUTE version 2 are each refused under a 3GPP
profile, naming the clause, and each remains available under
Unprofiled. The gzip refusal is in #68 andcarries a specification contradiction; see that pull request and 5G-MAG/Standards#212.
Verification
T1: 67 cases passing on this branch. T2: the FDT captured off the wire by a sniffer that links no library code confirms
Transfer-LengthandFEC-OTI-FEC-Instance-IDabsent,File@Expirespresent and distinct fromFDT-Instance@Expires, and<schemaVersion>2</schemaVersion>correctly positioned for the profiled schema.Where this leaves conformance
A clause-by-clause sweep was done alongside this work: TS 26.346 clause 7.2, TS 26.346 annex L and
TS 26.517 clause 6.2 read as prose, and RFC 3926, RFC 3450 and RFC 3451 swept by extracting all 182
MUST and SHALL statements with their clause. With this pull request and the four built on it:
Ts26517Ts26346UnprofiledCongestion control has been split out into #98 and is not in this pull request. It is the last
mandatory item for
Unprofiled, it is written but unverified, and it closes no issue. Keeping ithere would have made this pull request 22 files and 3,417 lines, and would have forced a reviewer to
accept an unverified congestion controller alongside conformance fixes that are ready.
It does not affect the two 3GPP profiles either way: TS 26.346 clause 7.2.4 excludes congestion
control for MBMS download, which is exactly why this library's single channel with C=0 and a zero
CCI is conformant there.
Two limits on how far that sweep should be trusted. The RFCs were swept by extracting MUST and SHALL
rather than read as prose, so an obligation carried only in prose could have been missed. And "should"
and "may" items were assessed but deliberately not implemented. Both are recorded in the conformance
record rather than glossed over.
Merge order
This one first. Every other pull request in the set is based on this branch
(
feature/wave3-rfc6726-compliance-fixes) rather than ondevelopment, so each of their diffs shows onlyits own commits and no fix in this set is presented for review twice. GitHub retargets them to
developmentautomatically as this merges.Full order: this, then #61, then #64. #65 and #68 need only this one and can go in any order after it.
Known overlap
PR #84 fixes the same defect as #76,
ctestat the build root discovering nothing. The commit here(
build: enable testing in the top-level list file so ctest sees the suite) predates it by five days andsits in the base of this whole stack, so removing it means rewriting all five branches. #84's
include(CTest)is the more idiomatic one-liner. One of the two should be closed; whichever is kept,please close the other rather than merging both, since they touch the same lines and will conflict. A note
to that effect has been left on #84.
Later additions
Two commits were added after the first review round, both from re-checking this branch against the
issues it closes rather than from new review comments.
fdt: wrap the FDT Instance ID to 0, as RFC 3926 defines the sequence. The branch had implementedRFC 6726's rule, reusing the smallest expired identifier and throwing when none had expired. RFC 3926
clause 3.4.1 states the sequence unconditionally and gives it no failure case, so a sender could refuse
to continue in a situation the specification it implements does not treat as an error. Issue TSI is truncated to 16 bits, and the FDT Instance ID never wraps #73 and
its description have been corrected the same way.
receiver: keep a file bootstrapped from EXT_FTI when the FDT completes. The bootstrap Packets for a TOI are discarded when they arrive before the FDT that describes them #74 asks forwas being undone: the replacement sweep that de-duplicates by
Content-Locationcompared thebootstrapped file's empty placeholder against the FDT's own, and erased the bootstrapped file every
time the FDT completed. Found by adding the coverage Packets for a TOI are discarded when they arrive before the FDT that describes them #74 had none of.
ipsec: authenticate the ESP association as well as encrypting it, closing IPsec sets up an encryption-only ESP association, with no packet authentication #96. The associationinstalled by
enable_ipsec()carriedXFRMA_ALG_CRYPTand nothing else, so it offeredconfidentiality without integrity.
transmitter: build an IPv6 header on the tunnel path, and budget for it, closing The UDP tunnel send path is IPv4-only and throws on an IPv6 destination #97. The tunnelpath took its addresses with
to_v4(), which throws for a v6 address, and budgeted a 20-byte IPheader regardless of the family in use.
transmitter: send to the announced destination as well as through the tunnel. No issue: thischange was already written, but it was sitting inside RFC 5053 Raptor FEC scheme: codec, FLUTE integration, and the missing-symbol accessor #61's Raptor integration commit, which is
not where a change to the send path belongs and meant the tunnel send path was being altered by
two pull requests at once. It is moved here, where The send socket is not bound to the configured source address when a tunnel is set #82 and The UDP tunnel send path is IPv4-only and throws on an IPv6 destination #97 already live, and removed from
RFC 5053 Raptor FEC scheme: codec, FLUTE integration, and the missing-symbol accessor #61. With a tunnel configured the announced destination now receives the plain copy as well, so
a receiver joining that address directly is no longer left with nothing. This is also the
behaviour the The send socket is not bound to the configured source address when a tunnel is set #82 commit above was written for: binding the source address matters because a
tunnelled session also emits an untunnelled copy, which until this commit it did not.
Four further commits, from reading RFC 5651 and RFC 5775
These came out of a prose read of the version 2 building blocks, RFC 5775 (ALC) and RFC 5651 (LCT),
undertaken to scope #81's second step. All three turned out to belong here rather than on the version 2
branch, because the provisions they rest on are in the version 1 documents too, or because this library
has one code path for both versions.
receiver: a packet carrying no payload is not an FDT packet. A packet with no payload restarted FDTreception and reassembled a zero-length object from it: a header with no TOI decodes to TOI 0, so such
a packet entered the TOI 0 branch. One datagram from a conformant peer ending its session does this,
and RFC 3926 clause 3.1 is what makes such a peer send one.
RFC 3450 clause 4.1:
RFC 3450 clause 4.1, on how a receiver tells:
RFC 3926 clause 3.1, on the shape a FLUTE sender uses:
Removing the guards makes the new test fail on the stray TOI 0 entry, so it tests the fix rather than
passing regardless.
alc: send the data-less Close Session packet the specification provides for.close_session()sets aflag on packets sent from that point on, so on a session with an empty queue it signalled nothing at
all: once the file set empties,
send_fdt()returns early and no further packet is ever built. Thepacket RFC 3450 clause 4.1 provides for is now sent, in the shape RFC 3926 clause 3.1 requires. A TSI
wider than 32 bits cannot express it, since dropping the TOI drops the half-word the two fields share;
that is refused and logged rather than emitting the TOI the clause forbids.
ipsec: select the session's traffic by protocol and port, not the destination alone. The policyselector named only the destination address, so it captured every datagram addressed to that group
whatever it was for.
RFC 5775 clause 5.1.1:
RFC 5775 clause 5.1.2.1:
This one is strictly a version 2 obligation and it still lands here. RFC 3450 contains no IPsec
baseline at all; the baseline arrives with RFC 5775 and reaches FLUTE through RFC 6726 clause 7.5. But
this library has one
enable_ipsec()and no per-version IPsec path, so the over-broad selector wasover-broad for both versions. For version 1 this is a defect fix; for version 2 it is also a
conformance fix.
enable_esp()gains a port parameter and both callers pass the port they alreadyhold.
T2, since installing a policy needs CAP_NET_ADMIN and so has no test scaffolding. Built against the
running kernel and read back:
Reproducible with a three-line program calling
LibFlute::IpSec::enable_esp(0x1234, "239.255.42.7", 40085, Direction::Out, key, auth_key). Testpolicies and states were flushed afterwards.
tests: quote the field definition the IPv6 header test checks against. Comment and test-messageonly, no assertion changed.
tests/test_transmitter.cppnamed RFC 8200 clause 3 in a comment andthen asserted against it with an unquoted gtest failure message on the next line, which
tools/verify-citations.pyreads as the clause's text and cannot confirm. It now sets out thesentence:
RFC 8200 clause 3:
Found by running the checker over every source file on every branch rather than only the files each
change touched. It was reporting one unconfirmed citation on all six branches, and had been for as
long as that test has existed.